Fix Container::pending_sort tracking
#93008
Conversation
When Container::queue_sort() is called, pending_sort is set to true to indicate when a call to _sort_children() is queued, to avoid queueing multiple calls. Container::_sort_children() sets pending_sort back to false when finished, but did not do this when the container was not inside the tree. This would allow cases where queue_sort() could be called just before removing from the tree, causing _sort_children() to never reset pending_sort, preventing any future queue_sort() calls from queueing again. One case where this happened was with the "Saving Scene" progress bar in the editor - when saving for the first time (or the first time the progress bar popup otherwise appeared in the editor), _sort_children() would be called successfully. After the progress bar popup was hidden, then shown again on future saves, _sort_children() would not be called again, resulting in the progress bar not taking up as much space as it should. This issue used to be avoided by setting pending_sort to false immediately on NOTIFICATION_ENTER_TREE - however, this would allow multiple calls to be queued at the same time when entering the tree (godotengine#92644). The multiple calls was fixed recently by removing this assignment, but this also made possible the case where pending_sort is never reset. This change sets pending_sort back to false in _sort_children() whether or not it's in the tree. Since this is done in a deferred call, it should still avoid the previous issue of multiple calls being queued at once on entering the tree. Fixes godotengine#92971
|
CC @WhalesState |
|
Thanks! |
|
I believe this is the correct fix instead of reverting the changes. anyway, this was able to fix the saving scene issue without forcing all the Containers in the project to call sort_children twice when the project starts.
|
|
Just want to clarify, this change does not revert #92645 - I just mentioned the previous issue to call out that we don't want to undo the fix for it. The difference from before the previous PR is that now The |
|
I was really confused, thought that my changes was reverted. |
|
This didn't work, already VISIBILITY CHANGED Maybe adding |
When
Container::queue_sort()is called,pending_sortis set to true to indicate when a call to_sort_children()is queued, to avoid queueing multiple calls.Container::_sort_children()setspending_sortback to false when finished, but did not do this when the container was not inside the tree. This would allow cases wherequeue_sort()could be called just before removing from the tree, causing_sort_children()to never resetpending_sort, preventing any futurequeue_sort()calls from queueing again.One case where this happened was with the "Saving Scene" progress bar in the editor - when saving for the first time (or the first time the progress bar popup otherwise appeared in the editor),
_sort_children()would be called successfully. After the progress bar popup was hidden, then shown again on future saves,_sort_children()would not be called again, resulting in the progress bar not taking up as much space as it should.This issue used to be avoided by setting
pending_sortto false immediately on NOTIFICATION_ENTER_TREE - however, this would allow multiple calls to be queued at the same time when entering the tree (#92644). The multiple calls was fixed recently by removing this assignment, but this also made possible the case wherepending_sortis never reset.This change sets
pending_sortback to false in_sort_children()whether or not it's in the tree. Since this is done in a deferred call, it should still avoid the previous issue of multiple calls being queued at once on entering the tree.Fixes #92971